Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Install Jinja2 templates for click_completion #2422

Merged
merged 1 commit into from
Jun 26, 2018

Conversation

wjv
Copy link
Contributor

@wjv wjv commented Jun 26, 2018

The new version of click_completion requires its Jinja2 templates to be installed along with the package in order to work. If not, any invocation of pipenv --completion results in a jinja2.exceptions.TemplateNotFound for the template in question.

I’ve just trivially added the pattern to setup.py, and tested that it works.

@uranusjr
Copy link
Member

@techalchemy We’ll need another release very soon 🤦‍♂️

@uranusjr uranusjr added the PR: awaiting-merge The PR related to this issue has been reviewed and is awaiting merge. label Jun 26, 2018
@techalchemy
Copy link
Member

Today is the plan

@ilovezfs
Copy link

Is this sufficient to make sure they end up in the sdist too?

@uranusjr
Copy link
Member

@ilovezfs It should, I think.

@daogilvie
Copy link

daogilvie commented Jun 29, 2018

@ilovezfs @uranusjr Hi — sadly it looks like the latest commit of master, with this PR merged, does not include the files in the sdist under setuptools 39.0.1, as I am just discovering now 😂 . I don't really grok Python packaging sometimes, but I have found adding include pipenv/vendor/click_completion/*.j2 to the MANIFEST.in solves the problem. I can reopen an issue and/or make a PR, but I don't want to just assume you are happy with the solution of adding to the MANIFEST.in file.

For completeness I have tried CPython 3.6.5 and 2.7.15 on MacOS high sierra. The sdist as generated there is affected, as is the one installed by homebrew on my system.

Please advise if I'm somehow using a bad branch, but the merge commit for this PR is in the git log so I'm pretty sure it's in there.

@techalchemy
Copy link
Member

We didn’t actually release an sdist yet but I would believe that our manifest doesn’t include these. Feel free to PR a fix and thanks for the heads up!

@techalchemy techalchemy added this to the 2018.7.0 milestone Jul 1, 2018
@wjv wjv deleted the fix_completion branch August 13, 2018 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: awaiting-merge The PR related to this issue has been reviewed and is awaiting merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants